Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(uplink): Ensure string input for b64enc in secret.yaml #686

Merged
merged 14 commits into from
Sep 12, 2023
Merged

fix(uplink): Ensure string input for b64enc in secret.yaml #686

merged 14 commits into from
Sep 12, 2023

Conversation

Sathwik-git
Copy link
Contributor

This PR fixes the uplink chart issue and added unit tests
Fixes #664

@dduportal dduportal self-assigned this Sep 8, 2023
dduportal
dduportal requested changes Sep 8, 2023
View reviewed changes
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for this proposal!

It is a good direction but it still requires some work:

  • Please set up a proper name for this PR. THe sentence What does this PR do? does not indicates the purpose of this pull request. You might want to use https://www.conventionalcommits.org/en/v1.0.0/ as a tool to help you phrase the title

  • You are not solving the problem described in Fix uplink chart issue: invalid value; expected string #664 . If you follow the issue instruction (running the command helm template for the uplink chart) you will see that your change does not fix the error message:

    helm-charts git:(Sathwik-git/main)$ helm template charts/uplink
Error: template: uplink/templates/secret.yaml:8:47: executing "uplink/templates/secret.yaml" at <b64enc>: invalid value; expected string

    I've added a suggestion about your unit test explaining how to simulate this behavior in the tests to give you a direction.

charts/uplink/templates/tests/secret_test.yaml Outdated Show resolved Hide resolved
charts/uplink/templates/tests/secret_test.yaml Outdated Show resolved Hide resolved
@Sathwik-git Sathwik-git changed the title What does this PR do? fix(uplink): Ensure string input for b64enc in secret.yaml Sep 8, 2023
@Sathwik-git
Copy link
Contributor Author

I hope it works now

Sathwik-git
Sathwik-git commented Sep 10, 2023
View reviewed changes
Copy link
Contributor Author

@Sathwik-git Sathwik-git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes made as per the request

@dduportal dduportal added bug Something isn't working uplink labels Sep 12, 2023
dduportal
dduportal approved these changes Sep 12, 2023
View reviewed changes
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks for the contribution!

LGTM

@dduportal dduportal merged commit 5c24e03 into jenkins-infra:main Sep 12, 2023
@dduportal
Copy link
Contributor

For info, it looks like this change failed to be deployed in the jenkins-infra public cluster with the following error:

in clusters/publick8s.yaml: command "/usr/local/bin/helm" exited with non-zero status:

PATH:
  /usr/local/bin/helm

ARGS:
  0: helm (4 bytes)
  1: diff (4 bytes)
  2: upgrade (7 bytes)
  3: --allow-unreleased (18 bytes)
  4: uplink (6 bytes)
  5: jenkins-infra/uplink (20 bytes)
  6: --version (9 bytes)
  7: 0.1.3 (5 bytes)
  8: --namespace (11 bytes)
  9: uplink (6 bytes)
  10: --values (8 bytes)
  11: /tmp/helmfile1465312871/uplink-uplink-values-6cf6f995bf (55 bytes)
  12: --values (8 bytes)
  13: /tmp/helmfile1103962376/uplink-uplink-values-6b6c489b89 (55 bytes)
  14: --suppress-secrets (18 bytes)
  15: --context (9 bytes)
  16: 2 (1 bytes)
  17: --reset-values (14 bytes)

ERROR:
  exit status 1

EXIT STATUS
  1

STDERR:
  Error: Failed to render chart: exit status 1: Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: [apiVersion not set, kind not set]
  Error: plugin "diff" exited with error

COMBINED OUTPUT:
  Error: Failed to render chart: exit status 1: Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: [apiVersion not set, kind not set]
  Error: plugin "diff" exited with error

The setup is visible here:

postgresql:
    url: postgres://<redacted>@<redacted>:<redacted>@<redacted>:5432/uplink
client:
    id: <redacted>
    secret: <redacted>
sentry:
    dsn: https://<redacted>@sentry.io/<redacted>

@dduportal dduportal mentioned this pull request Sep 12, 2023
Merged
@dduportal
Copy link
Contributor

Hello @Sathwik-git , if you have time to help us on this problem that would be wonderful!

This was referenced Sep 13, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dduportal dduportal dduportal approved these changes

Assignees

@dduportal dduportal

Labels
bug Something isn't working uplink
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix uplink chart issue: invalid value; expected string
2 participants
@Sathwik-git @dduportal